fix missing types and jsDocs #89

erikyo · 2024-05-05T22:32:58Z

This pull request addresses the need for consistent documentation within the repository by adding JSDoc comments where they were previously missing.

This enhances code readability and maintainability.

Each file has been reviewed and updated as necessary to ensure comprehensive documentation coverage.

erikyo · 2024-05-08T19:44:55Z

the last commit was required to show the IDE tooltip / suggestion in the right way (tested vscode and webstorm). replaced the default export with named export for Po* singleton

erikyo · 2024-05-10T20:02:38Z

I enabled strict types and fails

cc. @johnhooks

johnhooks

@erikyo looking good, my overall suggestion is to only attempt adding the initial set of types, not fixing logical or type errors. It would be nice to do those one at a time in separate PRs so they can get the attention they deserve.

src/mocompiler.js

johnhooks · 2024-05-11T13:21:39Z

src/mocompiler.js

+ * @param {import('./types.js').GetTextTranslations['translations']} translations
+ * @return {import('./types.js').GetTextTranslations['translations']}


If you use the suggestion above, I would also declare the Translations type like this:

// At the top of the file with the other type import. We can use `GetTextTranslations` directly because it's already imported. /** * @param {GetTextTranslations['translations']} Translations */

Suggested change

* @param {import('./types.js').GetTextTranslations['translations']} translations

* @return {import('./types.js').GetTextTranslations['translations']}

* @param {Translations} translations

* @return {Translations}

I haven't checked, though if Translations could be typed separately, rather than looking it up with key access on GetTextTranslations, that would be preferred. If that is acceptable then it would change to:

// At the top of the file with the other type import. We can use `GetTextTranslations` directly because it's already imported. /** * @typedef {import('./types.js').Translations) Translations */

yes be patient but that is the fastest way with my IDE to generate jsdocs types. a 'cleaner' commit will follow all these changes

i just give a try and seems that jsDocs only supports inline import :/

It looks to me like you didn't import them. Is it okay for me to PR this branch so you can see what I mean?

sure! I think you have even permission to edit my repo, if not tell me

src/mocompiler.js

src/pocompiler.js

src/shared.js

johnhooks · 2024-05-11T13:55:03Z

src/types.js

+ * @property {number} [foldLength] the fold length.
+ * @property {boolean} [escapeCharacters] Whether to escape characters.
+ * @property {boolean} [sort] Whether to sort messages.
+ * @property {string} [eol] End of line character.


I would declare all the types in a types.ts file, I think that is pretty standard practice when using JSDocs.

i would suggest a PR after this one that stabilizes imports and sorting for jsDocs. in this one I would feel satisfied if I can reduce the errors to zero :😇

however, I do not know if it is a good practice or not to create a '.ts' for declarations, but I think there would be problems with running tests for example, it would force one to use tsnode to execute the suite

johnhooks · 2024-05-11T14:14:24Z

src/mocompiler.js


  this._translations = [];

-  this._writeFunc = 'writeUInt32LE';


Can this be removed?

That assignment appears to have been established at some point, presumably intended for modification through options. However, with no implementation efforts undertaken, it remains as unused memory.

https://nodejs.org/api/buffer.html#bufwriteuint32levalue-offset

These serve as the 'placeholders' for the associated function name utilized for reading bytes from the po/mo files. Frankly, I'm uncertain whether it's worthwhile to implement the others, as file writing should adhere to a similar approach.

this made typing more complicated, what do you say I leave it or is it OK?

I would argue that byte order should be an option of the compiler, so rather than remove the concept of changing the write function we should fix it.

If a mo file is parsed in a specific byte order it should be rewritten in the same order.

should be an option of the compiler

i think this requires a separate issue. won't fix in this pr

src/mocompiler.js

Co-authored-by: John Hooks <[email protected]>

erikyo · 2024-05-11T15:07:11Z

will apply the suggestion asap... thank you John! Anyhow check this:

https://github.com/smhg/gettext-parser/pull/89/checks

there are only about 30 type errors left 🚀 then we are good to go

erikyo · 2024-05-11T19:42:08Z

The remaining types I sincerely believe require something structural that cannot be done in this pr.

I am referring for example to these which should be classes (

gettext-parser/src/poparser.js

Line 590 in babf333

util.inherits(PoParserTransform, Transform);

) which has to extend the base class to inherit the types or objects which have the same keys (ie the translation) but they have been used for different ways creating some confusion in the type to be used

what do you think @johnhooks? should I disable the type checking in order to pass again the tests so then we can focus on the last 20-30 issues?

johnhooks · 2024-05-11T23:56:56Z

@erikyo I think it's fair to set continue-on-error: true on the tsc GitHub action step, and accept that it fails. There might also be other ways to indicate a step as an optional part of the job.

erikyo · 2024-05-12T00:06:40Z

in this case we should split the CI actions into 2 different steps because otherwise it will allow the tests to fail

erikyo · 2024-05-12T00:31:40Z

fixed, and fixed the reporter in the code tab (previously was duplicated)

This commit adds missing types and attempts fix type errors. There are still a few type errors, though how to fix them is not clear. Adds the `Translations` type for the `translations` property of the `GetTextTranslations` type.

fix: adjust typing of the parsers and compilers

johnhooks

Looks great @erikyo, very exciting to get the type errors resolved and have the declarations output for exporting first party types.

fix missing types and jsDocs

42a9693

erikyo mentioned this pull request May 5, 2024

Repository Refactoring (proposal) #82

Open

10 tasks

erikyo marked this pull request as draft May 5, 2024 22:37

erikyo mentioned this pull request May 6, 2024

v8.1.0 Update #86

Open

erikyo added 2 commits May 8, 2024 21:20

removing definition file, the definitions were moved to the type.js file

cbe6e8f

enhanced type for pocompiler and postream

a9e6fa0

enhancing js docs types

7f94d00

erikyo force-pushed the jsDocs-fix branch from ed87d7b to 85121d6 Compare May 10, 2024 20:06

jsdocs types (still few error to solve)

f4366dd

erikyo force-pushed the jsDocs-fix branch from 608e28b to f4366dd Compare May 11, 2024 12:28

johnhooks reviewed May 11, 2024

View reviewed changes

src/mocompiler.js Outdated Show resolved Hide resolved

erikyo and others added 2 commits May 11, 2024 16:47

@johnhooks review suggestions

977605c

Co-authored-by: John Hooks <[email protected]>

apply suggestion by johnhooks

4262c6d

Co-authored-by: John Hooks <[email protected]>

erikyo added 2 commits May 11, 2024 18:24

apply @johnhooks suggestions 🙌

002ae75

wip types

babf333

fixed types

7dc97d7

erikyo requested a review from johnhooks May 11, 2024 23:18

erikyo force-pushed the jsDocs-fix branch 3 times, most recently from 42498d6 to 981551b Compare May 12, 2024 00:27

allows tsc to fail in ci tests

aad7d48

erikyo force-pushed the jsDocs-fix branch from 981551b to aad7d48 Compare May 12, 2024 00:29

johnhooks and others added 6 commits May 11, 2024 22:14

fix: adjust typing of the parsers and compilers

d242a94

This commit adds missing types and attempts fix type errors. There are still a few type errors, though how to fix them is not clear. Adds the `Translations` type for the `translations` property of the `GetTextTranslations` type.

add imports for types

6b3b940

add encoding declaration

c072f28

add types directory to tsconfig include

9cf82f2

remove types directory from .gitignore

b78e19e

Merge pull request #2 from johnhooks/fix/adjust-typing

0771cf9

fix: adjust typing of the parsers and compilers

erikyo marked this pull request as ready for review May 12, 2024 19:01

erikyo merged commit c40a44c into smhg:development May 12, 2024
5 checks passed

johnhooks approved these changes May 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix missing types and jsDocs #89

fix missing types and jsDocs #89

erikyo commented May 5, 2024

erikyo commented May 8, 2024

erikyo commented May 10, 2024

johnhooks left a comment

johnhooks May 11, 2024

erikyo May 11, 2024

erikyo May 11, 2024

johnhooks May 11, 2024

erikyo May 11, 2024

johnhooks May 11, 2024

erikyo May 11, 2024

erikyo May 11, 2024

johnhooks May 11, 2024

erikyo May 11, 2024

johnhooks May 11, 2024

erikyo May 11, 2024

erikyo commented May 11, 2024

erikyo commented May 11, 2024

johnhooks commented May 11, 2024

erikyo commented May 12, 2024

erikyo commented May 12, 2024

johnhooks left a comment

		* @param {import('./types.js').GetTextTranslations['translations']} translations
		* @return {import('./types.js').GetTextTranslations['translations']}

fix missing types and jsDocs #89

fix missing types and jsDocs #89

Conversation

erikyo commented May 5, 2024

erikyo commented May 8, 2024

erikyo commented May 10, 2024

johnhooks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikyo commented May 11, 2024

erikyo commented May 11, 2024

johnhooks commented May 11, 2024

erikyo commented May 12, 2024

erikyo commented May 12, 2024

johnhooks left a comment

Choose a reason for hiding this comment